Bug 1685338: pkg/cvo: Reason granularity for RemoteFailed #268
Conversation
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
@wking: This pull request references Bugzilla bug 1685338, which is invalid:
Comment DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
approved
We want to be able to distinguish these conditions, which can be due to internal misconfiguration or external Cincinnati/network errors [1]. The former can be fixed by cluster admins. The latter could go either way. I dropped the len(upstream) guard from checkForUpdate because there's already an earlier guard in syncAvailableUpdates. The guard I'm removing is from db150e6 (cvo: Perform status updates in a single thread, 2018-11-03, openshift#45). The covering guard is from the later 286641d (api: Update to objects from openshift/api, 2018-11-15, openshift#55). Personally, I'd rather have GetUpdates return an *Error, so we could dispense with the cast and unused Unknown-reason fallback. But Abhinav wanted the explicit cast in return for a more familiar error type [2]. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1685338 [2]: openshift#268 (comment)
wking
force-pushed
the
get-updates-error-granularity
branch
from
363a762 to
8772ab1
Compare
pkg/cvo/availableupdates.go
Outdated
| if err != nil { | ||
| return nil, configv1.ClusterOperatorStatusCondition{ | ||
| Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "InvalidID", | ||
| Message: fmt.Sprintf("invalid cluster ID: %v", err), |
|
except #268 (comment) |
We want to be able to distinguish these conditions, which can be due to internal misconfiguration or external Cincinnati/network errors [1]. The former can be fixed by cluster admins. The latter could go either way. I dropped the len(upstream) guard from checkForUpdate because there's already an earlier guard in syncAvailableUpdates. The guard I'm removing is from db150e6 (cvo: Perform status updates in a single thread, 2018-11-03, openshift#45). The covering guard is from the later 286641d (api: Update to objects from openshift/api, 2018-11-15, openshift#55). Personally, I'd rather have GetUpdates return an *Error, so we could dispense with the cast and unused Unknown-reason fallback. But Abhinav wanted the explicit cast in return for a more familiar error type [2]. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1685338 [2]: openshift#268 (comment)
Moving it up one level into calculateAvailableUpdatesStatus places it right next to the existing len(upstream) check, so now both "is the upstream URI valid?" checks are in the same place.
wking
force-pushed
the
get-updates-error-granularity
branch
from
bff5982 to
dca2686
Compare
|
/bugzilla refresh |
|
@wking: This pull request references Bugzilla bug 1685338, which is invalid:
Comment DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/valid-bug
|
@abhinavdahiya: This pull request references Bugzilla bug 1685338, which is valid. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@wking: All pull requests linked via external trackers have merged. Bugzilla bug 1685338 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4 participants
We want to be able to distinguish these conditions, which can be due to internal misconfiguration or external Cincinnati/network errors. The former can be fixed by cluster admins. The latter could go either way.
I dropped the
len(upstream)guard fromcheckForUpdatebecause there's already an earlier guard insyncAvailableUpdates. The guard I'm removing is from db150e6 (#45). The covering guard is from the later 286641d (#55).